Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply item mods to across modules #933

Merged
merged 1 commit into from
Nov 13, 2021
Merged

Conversation

Pugzy
Copy link
Contributor

@Pugzy Pugzy commented Oct 31, 2021

The Issue

Maps that use item mods can cause items (typically arrows and gapples) to be non-stackable.
If the item mod is applied to the items in the player's inventory but not on items picked up or given to them they will not stack.

Background on Item Mods

Item mods work by checking all items to see if modifications are required, once checked all items are marked with meta (in the current case hidden lore). Items that are not checked and are received by a player are missing this extra data so the items do not match.

The Solution(s)

There are a few elements fixed/changed to make this function as expected.

  • Item mods rules applied to parsed kits inside the kill rewards and block drop modules.
  • Adding an extra check for picking up arrows inside item mods (arrow pickup event doesn't exist in this version of Bukkit).
  • Allowing the modification of picked up arrows by removing the arrow (faking the pickup) and giving the correct item.
  • Applying that same logic to the ToolRepairMatchModule so items don't just disappear but look to be picked up (with the animation and world sound unlike before).

Thanks to @Pablete1234 and @PabloPintor for debugging and testing.

Signed-off-by: Pugzy [email protected]

@Pugzy Pugzy requested a review from Electroid as a code owner October 31, 2021 19:51
@Pugzy Pugzy force-pushed the item-mod-fixes branch 2 times, most recently from 656d99f to 8b2d343 Compare November 4, 2021 21:44
Comment on lines +68 to +69
final ItemStack itemStack = factory.getKits().parseItem(itemEl, false);
itemModifier.ifPresent(imm -> imm.applyRules(itemStack));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal of ItemModifyModule is to ensure that all ItemStacks are modified to certain rules, I would think that parseItem should return the already modified item? The problem here is that the caller has to remember to call applyRules, which would be quite easy to forget.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original pgm did it like this and i don't know why, but something tells me it's better not to.

I feel there may be use-cases for parseItem that don't require itemmodding

@Pablete1234 Pablete1234 added bug Something isn't working ready PR is ready to merge labels Nov 12, 2021
@Electroid Electroid merged commit 6ccf8f4 into PGMDev:dev Nov 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PR is ready to merge
Development

Successfully merging this pull request may close these issues.

3 participants